feat(Slider): add support for custom tooltip content#12531
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds optional ChangesSlider Custom Tooltip
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Slider
participant Thumb
participant Tooltip
Caller->>Slider: Pass tooltipContent and thumbAriaValueText
Slider->>Thumb: Set aria-valuetext override
Slider->>Tooltip: Set custom content when tooltip is enabled
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-core/src/components/Slider/Slider.tsx (1)
479-483: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a nullish check for
tooltipContent
tooltipContentis typed asReact.ReactNode, so0and''get ignored here and the tooltip falls back tofindAriaTextValue(). UsetooltipContent !== undefined(or??) instead of a truthiness check.Proposed fix
- content={tooltipContent ? tooltipContent : findAriaTextValue()} + content={tooltipContent !== undefined ? tooltipContent : findAriaTextValue()}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/components/Slider/Slider.tsx` around lines 479 - 483, The Tooltip content selection in Slider should not use a truthiness check for tooltipContent, since React.ReactNode can legitimately be 0 or an empty string. Update the Tooltip rendering logic in Slider to use a nullish check (for example, tooltipContent !== undefined or the nullish coalescing pattern) so Tooltip receives tooltipContent whenever it is defined, and only falls back to findAriaTextValue() when tooltipContent is actually undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/react-core/src/components/Slider/Slider.tsx`:
- Around line 479-483: The Tooltip content selection in Slider should not use a
truthiness check for tooltipContent, since React.ReactNode can legitimately be 0
or an empty string. Update the Tooltip rendering logic in Slider to use a
nullish check (for example, tooltipContent !== undefined or the nullish
coalescing pattern) so Tooltip receives tooltipContent whenever it is defined,
and only falls back to findAriaTextValue() when tooltipContent is actually
undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4240228-16d6-425e-9418-0b581c9cb8f4
📒 Files selected for processing (4)
packages/react-core/src/components/Slider/Slider.tsxpackages/react-core/src/components/Slider/__tests__/Slider.test.tsxpackages/react-core/src/components/Slider/examples/Slider.mdpackages/react-core/src/components/Slider/examples/SliderCustomTooltip.tsx
|
Preview: https://pf-react-pr-12531.surge.sh A11y report: https://pf-react-pr-12531-a11y.surge.sh |
| aria-valuetext={findAriaTextValue()} | ||
| aria-label={thumbAriaLabel} | ||
| aria-valuetext={thumbAriaValueText ? thumbAriaValueText : findAriaTextValue()} | ||
| aria-label={thumbAriaLabel ? thumbAriaLabel : thumbAriaLabel} |
There was a problem hiding this comment.
Did you mean to use a different value for one of these?
There was a problem hiding this comment.
I can't remember, I may have been thinking the default needed to be updated by the new prop but it doesn't. Reverted.
| <Slider | ||
| hasTooltipOverThumb | ||
| tooltipContent={customTooltipContent()} | ||
| value={value} | ||
| onChange={(_event: SliderOnChangeEvent, value: number) => setValue(value)} | ||
| customSteps={steps} | ||
| /> |
There was a problem hiding this comment.
May not be absolutely necessary with the above example verbiage added in, but we could add a second slider tothis example showing an implementation more similar to the original issue, where each step is a date timestamp. If we do, maybe we have the slider steps min and max values be 1-10 with whole number intervals
0255625 to
b1d6f96
Compare
3 participants
What: Closes #12160
tooltipContent, which overrides the default behavior of the current value for a custom tooltipSummary by CodeRabbit
thumbAriaValueTextandtooltipContentto customize slider thumb accessibility text and tooltip text.tooltipContent,thumbAriaValueText, and recommendedtooltipProps.aria-valuetextnow use the custom values when provided, falling back to the value-derived text.aria-valuetextbehavior with the new props.